[SPARK-27754][K8S] Introduce additional config (spark.kubernetes.driver.request.cores) for driver request cores for spark on k8s #24630

arunmahadevan · 2019-05-16T20:37:51Z

What changes were proposed in this pull request?

Spark on k8s supports config for specifying the executor cpu requests
(spark.kubernetes.executor.request.cores) but a similar config is missing
for the driver. Instead, currently spark.driver.cores value is used for integer value.

Although pod spec can have cpu for the fine-grained control like the following, this PR proposes additional configuration spark.kubernetes.driver.request.cores for driver request cores.

resources:
  requests:
    memory: "64Mi"
    cpu: "250m"

How was this patch tested?

Unit tests

Spark on k8s supports config for specifying the executor cpu requests (spark.kubernetes.executor.request.cores) but a similar config is missing for the driver. Apparently `spark.driver.cores` works but its not evident that this accepts fractional values (its defined as an Integer config but apparently accepts decimals). To keep in sync with the executor config a similar driver config can be introduced (spark.kubernetes.driver.request.cores) for explicitly specifying the driver CPU requests. If not provided, the value will default to `spark.driver.cores` as before.

SparkQA · 2019-05-16T20:50:30Z

Test build #105472 has finished for PR 24630 at commit 4002b74.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

felixcheung

LGTM

felixcheung · 2019-05-17T01:24:35Z

@mccheah ?

dongjoon-hyun · 2019-05-17T03:22:39Z

...rnetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala

+    conf.get(KUBERNETES_DRIVER_REQUEST_CORES).get
+  } else {
+    driverCpuCores
+  }


Thank you for making a PR, @arunmahadevan . Could you rewrite like the following one-liner?

- private val driverCoresRequest = if (conf.contains(KUBERNETES_DRIVER_REQUEST_CORES)) { - conf.get(KUBERNETES_DRIVER_REQUEST_CORES).get - } else { - driverCpuCores - } + private val driverCoresRequest = conf.get(KUBERNETES_DRIVER_REQUEST_CORES.key, driverCpuCores)

Thanks for the suggestion. I just followed the pattern used somewhere else in the code.

docs/running-on-kubernetes.md

dongjoon-hyun · 2019-05-17T03:32:27Z

...s/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStepSuite.scala

+      .configurePod(basePod)
+      .container.getResources
+      .getRequests.asScala
+    assert(requests1("cpu").getAmount === "1")


Can we avoid this assumption? You had better get the default value of DRIVER_CORES and compare with that.

IIUC, as per the existing logic the default value would be "1" if spark.driver.cores is not set and not the default value of spark.driver.cores which also happens to be 1. I did not want to change that logic.

@arunmahadevan . What I meant was this constant assertion, === "1". If we change the default value of that configuration, this test will fails. We had better get and use the default value from the conf.

@dongjoon-hyun , the logic in BasicDriverFeatureStep
private val driverCpuCores = conf.get(DRIVER_CORES.key, "1")
, sets the value of driverCpuCores to "1" if spark.driver.cores is not set in the spark conf. i.e. we don't use the DRIVER_CORES.defaultValue there. Let me know if my understanding is correct.
If so I cannot do something like assert(requests1("cpu").getAmount === DRIVER_CORES.defaultValue) here (assume this is what you are suggesting).

That should be the following because we should not have a magic number in the code.

- private val driverCpuCores = conf.get(DRIVER_CORES.key, "1") + private val driverCpuCores = conf.get(DRIVER_CORES)

val driverCpuQuantity = new QuantityBuilder(false) - .withAmount(driverCpuCores) + .withAmount(driverCpuCores.toString)

Please fix like the above. And don't use magic numbers.

dongjoon-hyun · 2019-05-17T03:35:37Z

...s/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStepSuite.scala

+      .configurePod(basePod)
+      .container.getResources
+      .getRequests.asScala
+    assert(requests3("cpu").getAmount === "100m")


If you don't mind, could you avoid repetitions like the following?

Seq("0.1", "100m").foreach { value => sparkConf.set(KUBERNETES_DRIVER_REQUEST_CORES, value) val requests = new BasicDriverFeatureStep(KubernetesTestConf.createDriverConf(sparkConf)) .configurePod(basePod) .container.getResources .getRequests.asScala assert(requests("cpu").getAmount === value) }

dongjoon-hyun · 2019-05-17T03:37:34Z

BTW, @arunmahadevan . The PR title, Introduce config for driver request cores, seems to claim too much. We already have a configuration, spark.driver.cores. And, it's intentionally designed like that. This PR should focus on the new benefits.

dongjoon-hyun

Although I don't know the history of the decision, I'll take a look again after revision. Thanks.

onursatici

It is worth noting that pod templates supports overriding spark container cpu requests: https://github.com/apache/spark/blob/master/docs/running-on-kubernetes.md#pod-template

If you pass a pod spec template with a single container in it with resource requests set, spark will use that as a base in building pod resources.

dongjoon-hyun · 2019-05-17T15:54:14Z

Ah, right. As @onursatici pointed out, pod spec can have cpu for the fine-grained control. (https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#meaning-of-memory).

   resources:
      requests:
        memory: "64Mi"
        cpu: "250m"

Since there is a more general way, the benefit of this PR is very limited. Please update the PR description, @arunmahadevan .

Or, could you enhance K8s document about this instead of adding this 3rd way of cpu spec? In fact, I prefer this document updates.

arunmahadevan · 2019-05-17T17:11:15Z

Updated title to Introduce additional config (spark.kubernetes.driver.request.cores) for driver request cores for spark on k8s

arunmahadevan · 2019-05-17T17:17:45Z

Or, could you enhance K8s document about this instead of adding this 3rd way of cpu spec? In fact, I prefer this document updates.

I agree we have an alternate way of specifying the driver cpu request cores for spark on k8s via the pod template. However we already have spark.kubernetes.executor.request.cores for executor but similar is missing for driver and it leads to confusion. Also users may just want to directly specify via the configs rather than providing pod template. So the point is then should we consider removing all the "spark.kubernetes" configs that can be specified via pod templates?

SparkQA · 2019-05-17T17:20:04Z

Test build #105500 has finished for PR 24630 at commit 8ca63d3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-05-18T02:51:36Z

I didn't suggest removing, @arunmahadevan . That kind of PR will be considered negatively too in the same way. You know, we usually are conservative for both sides (adding and removing).

So the point is then should we consider removing all the "spark.kubernetes" configs that can be specified via pod templates?

arunmahadevan · 2019-05-18T03:16:28Z

I didn't suggest removing, @arunmahadevan . That kind of PR will be considered negatively too in the same way. You know, we usually are conservative for both sides (adding and removing).

In this case I think we will get more consistent by introducing a config specific to k8s driver for which the executor config already exists.

SparkQA · 2019-05-18T03:29:38Z

Test build #105509 has finished for PR 24630 at commit ff688c2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

Thank you for updates, @arunmahadevan . I updated the PR description with #24630 (comment).

I'll wait for the other reviewers' comments.

dongjoon-hyun · 2019-05-19T04:27:48Z

Merged to master. Thank you, @arunmahadevan and @felixcheung .

arunmahadevan · 2019-05-19T08:55:42Z

Thank you @dongjoon-hyun for taking time to review and merge.

marxangels · 2019-08-18T07:37:13Z

Very inflexible，I'm looking for some config option to do such

resources:
  requests:
    memory: "911Mi"

maybe --conf spark.kubernetes.executor.request.memory=150Mi

Unfortunately, it doesn't work.

marxangels · 2019-08-18T07:39:24Z

This should be done through mapping, not one by one.

marxangels · 2019-08-18T08:51:29Z

The code is not elegant enough to support "unknown" k8s configuration items (why not use rule mapping instead of enumerating it one by one?).

…er.request.cores) for driver request cores for spark on k8s Spark on k8s supports config for specifying the executor cpu requests (spark.kubernetes.executor.request.cores) but a similar config is missing for the driver. Instead, currently `spark.driver.cores` value is used for integer value. Although `pod spec` can have `cpu` for the fine-grained control like the following, this PR proposes additional configuration `spark.kubernetes.driver.request.cores` for driver request cores. ``` resources: requests: memory: "64Mi" cpu: "250m" ``` Unit tests Closes apache#24630 from arunmahadevan/SPARK-27754. Authored-by: Arun Mahadevan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

felixcheung approved these changes May 17, 2019

View reviewed changes

dongjoon-hyun reviewed May 17, 2019

View reviewed changes

docs/running-on-kubernetes.md Show resolved Hide resolved

dongjoon-hyun reviewed May 17, 2019

View reviewed changes

dongjoon-hyun requested changes May 17, 2019

View reviewed changes

onursatici reviewed May 17, 2019

View reviewed changes

review comments and test case

8ca63d3

arunmahadevan changed the title ~~[SPARK-27754][K8S] Introduce config for driver request cores~~ [SPARK-27754][K8S] Introduce additional config (spark.kubernetes.driver.request.cores) for driver request cores for spark on k8s May 17, 2019

fix default value

ff688c2

dongjoon-hyun approved these changes May 18, 2019

View reviewed changes

dongjoon-hyun closed this in 1a8c093 May 19, 2019

Alexis-D mentioned this pull request Nov 16, 2020

[SPARK-27754][K8S] Introduce additional config (spark.kubernetes.driver.request.cores) for driver request cores for spark on k8s palantir/spark#716

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-27754][K8S] Introduce additional config (spark.kubernetes.driver.request.cores) for driver request cores for spark on k8s #24630

[SPARK-27754][K8S] Introduce additional config (spark.kubernetes.driver.request.cores) for driver request cores for spark on k8s #24630

arunmahadevan commented May 16, 2019 •

edited by dongjoon-hyun

Loading

SparkQA commented May 16, 2019

felixcheung left a comment

felixcheung commented May 17, 2019

dongjoon-hyun May 17, 2019

arunmahadevan May 17, 2019

dongjoon-hyun May 17, 2019

arunmahadevan May 17, 2019

dongjoon-hyun May 17, 2019

arunmahadevan May 17, 2019

dongjoon-hyun May 18, 2019

arunmahadevan May 18, 2019

dongjoon-hyun May 17, 2019 •

edited

Loading

arunmahadevan May 17, 2019

dongjoon-hyun commented May 17, 2019

dongjoon-hyun left a comment

onursatici left a comment

dongjoon-hyun commented May 17, 2019 •

edited

Loading

arunmahadevan commented May 17, 2019

arunmahadevan commented May 17, 2019

SparkQA commented May 17, 2019

dongjoon-hyun commented May 18, 2019

arunmahadevan commented May 18, 2019

SparkQA commented May 18, 2019

dongjoon-hyun left a comment •

edited

Loading

dongjoon-hyun commented May 19, 2019

arunmahadevan commented May 19, 2019

marxangels commented Aug 18, 2019

marxangels commented Aug 18, 2019

marxangels commented Aug 18, 2019

[SPARK-27754][K8S] Introduce additional config (spark.kubernetes.driver.request.cores) for driver request cores for spark on k8s #24630

[SPARK-27754][K8S] Introduce additional config (spark.kubernetes.driver.request.cores) for driver request cores for spark on k8s #24630

Conversation

arunmahadevan commented May 16, 2019 • edited by dongjoon-hyun Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented May 16, 2019

felixcheung left a comment

Choose a reason for hiding this comment

felixcheung commented May 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun May 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented May 17, 2019

dongjoon-hyun left a comment

Choose a reason for hiding this comment

onursatici left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented May 17, 2019 • edited Loading

arunmahadevan commented May 17, 2019

arunmahadevan commented May 17, 2019

SparkQA commented May 17, 2019

dongjoon-hyun commented May 18, 2019

arunmahadevan commented May 18, 2019

SparkQA commented May 18, 2019

dongjoon-hyun left a comment • edited Loading

Choose a reason for hiding this comment

dongjoon-hyun commented May 19, 2019

arunmahadevan commented May 19, 2019

marxangels commented Aug 18, 2019

marxangels commented Aug 18, 2019

marxangels commented Aug 18, 2019

arunmahadevan commented May 16, 2019 •

edited by dongjoon-hyun

Loading

dongjoon-hyun May 17, 2019 •

edited

Loading

dongjoon-hyun commented May 17, 2019 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading